Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add synchronous garbage collection #38676

Merged
merged 5 commits into from
Mar 1, 2017
Merged

Add synchronous garbage collection #38676

merged 5 commits into from
Mar 1, 2017

Conversation

caesarxuchao
Copy link
Member

@caesarxuchao caesarxuchao commented Dec 13, 2016 •
edited

Fix #29891.

Split into five commits:

  1. generated: don't need review
  2. API: got reviewed in API changes and registry changes for synchronous garbage collection #38678, i addressed @lavalamp's comments there.
  3. registry changes: @nikhiljindal could you help take a look?
  4. gc changes: reviewed by @deads2k in GC changes for synchronous GC supports #38679. It needs another pass.
  5. tests: @lavalamp @deads2k could take a look?

TODO:

Added foreground garbage collection: the owner object will not be deleted until all its dependents are deleted by the garbage collector. Please checkout the [user doc](https://kubernetes.io/docs/concepts/abstractions/controllers/garbage-collection/) for details.
deleteOptions.orphanDependents is going to be deprecated in 1.7. Please use deleteOptions.propagationPolicy instead.

@caesarxuchao caesarxuchao added the sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. label Dec 13, 2016
@caesarxuchao caesarxuchao self-assigned this Dec 13, 2016
@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Dec 13, 2016
@k8s-reviewable
Copy link

This change is Reviewable

@k8s-github-robot k8s-github-robot added kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. release-note-label-needed labels Dec 13, 2016
This was referenced Dec 13, 2016
Closed
Closed
@deads2k
Copy link
Contributor

deads2k commented Dec 13, 2016

Looks like all of pkg/controller/garbagecollector/garbagecollector.go was replaced. Can you open a pull with just the new files, no old changes, and I can review/merge that separately?

@deads2k
Copy link
Contributor

deads2k commented Dec 13, 2016

Looks like all of pkg/controller/garbagecollector/garbagecollector.go was replaced. Can you open a pull with just the new files, no old changes, and I can review/merge that separately?

ah, ok. I found the other pulls. I understand now.

@k8s-github-robot k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 13, 2016
@k8s-github-robot k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 15, 2017
@k8s-github-robot k8s-github-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API kind/old-docs labels Feb 16, 2017
@k8s-github-robot k8s-github-robot removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. kind/old-docs labels Feb 16, 2017
@caesarxuchao
Copy link
Member Author

Rebased. Reapplying the lgtm.

@caesarxuchao caesarxuchao added lgtm "Looks good to me", indicates that a PR is ready to be merged. and removed lgtm "Looks good to me", indicates that a PR is ready to be merged. labels Feb 28, 2017
@k8s-github-robot k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 28, 2017
@deads2k
Copy link
Contributor

deads2k commented Feb 28, 2017

Automatic merge from submit-queue

How did you manage to get this comment and not merge??

@deads2k
Copy link
Contributor

deads2k commented Feb 28, 2017

Also, check the errors for https://k8s-gubernator.appspot.com/build/kubernetes-jenkins/pr-logs/pull/38676/pull-kubernetes-e2e-gce-etcd3/20101/

@deads2k
Copy link
Contributor

deads2k commented Feb 28, 2017

Based on output, it's not a bug, but you need to log the actual error you got during the verification.

/go/src/k8s.io/kubernetes/_output/dockerized/go/src/k8s.io/kubernetes/test/e2e/garbage_collector.go:471
Feb 27 23:48:59.560: remaining rs post mortem: &v1beta1.ReplicaSetList{TypeMeta:v1.TypeMeta{Kind:"", APIVersion:""}, ListMeta:v1.ListMeta{SelfLink:"/apis/extensions/v1beta1/namespaces/e2e-tests-gc-7715r/replicasets", ResourceVersion:"12561"}, Items:[]v1beta1.ReplicaSet(nil)}
/go/src/k8s.io/kubernetes/_output/dockerized/go/src/k8s.io/kubernetes/test/e2e/garbage_collector.go:451

You did orphan it, but you covered up the actual error you got during verification

@deads2k
Copy link
Contributor

deads2k commented Feb 28, 2017

@k8s-bot gce etcd3 e2e test this

@caesarxuchao
Copy link
Member Author

Thanks for taking care of the tests. Added to the TODO to improve the log message of that test.

@k8s-github-robot k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 1, 2017
@k8s-github-robot k8s-github-robot removed lgtm "Looks good to me", indicates that a PR is ready to be merged. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Mar 1, 2017
@caesarxuchao caesarxuchao added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 1, 2017
@k8s-github-robot
Copy link

[APPROVALNOTIFIER] This PR is NOT APPROVED

The following people have approved this PR: caesarxuchao, k8s-merge-robot

Needs approval from an approver in each of these OWNERS Files:

We suggest the following people:
cc @lavalamp
You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

@caesarxuchao
Copy link
Member Author

Rebased 3 times after got the lgtm. Adding p2 label now.

@k8s-github-robot
Copy link

@k8s-bot test this [submit-queue is verifying that this PR is safe to merge]

@k8s-ci-robot
Copy link
Contributor

k8s-ci-robot commented Mar 1, 2017 •
edited

@caesarxuchao: The following test(s) failed:

Test name Commit Details Rerun command
Jenkins Cross Build 19326e34572bcbca68e63266b5d09474ed22a277 link @k8s-bot cross build this
Jenkins unit/integration 31cb266 link @k8s-bot unit test this

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

@k8s-github-robot
Copy link

Automatic merge from submit-queue (batch tested with PRs 38676, 41765, 42103, 41833, 41702)

@k8s-github-robot k8s-github-robot merged commit 3afefae into kubernetes:master Mar 1, 2017
@mborsz mborsz mentioned this pull request Nov 20, 2020
Closed
akhilerm pushed a commit to akhilerm/apimachinery that referenced this pull request Sep 20, 2022
@k8s-publishing-bot
Merge pull request #38676 from caesarxuchao/sync-gc
f6809c9 
Automatic merge from submit-queue (batch tested with PRs 38676, 41765, 42103, 41833, 41702)

Add synchronous garbage collection

Fix kubernetes/kubernetes#29891.

Split into five commits:
1. generated: don't need review
2. API: got reviewed in #38678, i addressed @lavalamp's comments there.
3. registry changes: @nikhiljindal could you help take a look?
4. gc changes: reviewed by @deads2k in #38679. It needs another pass.
5. tests: @lavalamp @deads2k could take a look?

TODO:
- [ ] Update doc. Note that the existing doc has been refactored in kubernetes/website#2488.
- [ ] add an admission controller to check if a user can set OwnerReference.BlockOwnerDeletion
- [ ] kubernetes/kubernetes#38676 (comment)
- [ ] split the unit tests garbagecollector_test.go according to the components tested.
- [ ] try if it's practically safe to use the cached object status in attempToDeleteItem(), after synchronous GC feature is stable. (Also see kubernetes/kubernetes#38676 (comment))
- [ ] add blockOwnerDeletion for rs adoption kubernetes/kubernetes#38679 (comment)
- [ ] https://k8s-gubernator.appspot.com/build/kubernetes-jenkins/pr-logs/pull/38676/pull-kubernetes-e2e-gce-etcd3/20101/ (improve the log message)

```release-note
Added foreground garbage collection: the owner object will not be deleted until all its dependents are deleted by the garbage collector. Please checkout the [user doc](https://kubernetes.io/docs/concepts/abstractions/controllers/garbage-collection/) for details.
deleteOptions.orphanDependents is going to be deprecated in 1.7. Please use deleteOptions.propagationPolicy instead.
```

Kubernetes-commit: 3afefae02a0588654693de27ef5387c1f9ab5f01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lavalamp lavalamp lavalamp left review comments

@nikhiljindal nikhiljindal nikhiljindal left review comments

Assignees

@lavalamp lavalamp

@deads2k deads2k

@nikhiljindal nikhiljindal

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note Denotes a PR that will be considered when it comes time to generate release notes. sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Milestone
v1.6
Development

Successfully merging this pull request may close these issues.

None yet
10 participants
@caesarxuchao @k8s-reviewable @deads2k @lavalamp @nikhiljindal @k8s-ci-robot @k8s-github-robot @madhusudancs @smarterclayton @thockin